Majority of the failures appear to be caused by:

from PIL import Image
ModuleNotFoundError: No module named 'PIL'

Is the import of new modules permitted? Or does the addition of this module spell death for this PR?

Let's not add a new dependency as BytesIO would be sufficient.

I'll have to take a deeper look at this be just skimming though it, it seems that there is going to be breaking changes. These will need to have a deprecation period. You can tell that you have breaking changes if the existing tests fail without any changes.

ETA: this is a very welcome change and I'd love to see this added.

Alright, sounds great. Would you like me to make changes and resubmit? Or are you going to make the corrections yourself? I haven't contributed to anything open-source before so I'm unsure of guidelines surrounding this sort of thing.

You got a great start! There is a few options here:

• replace the existing image path parameter (like you've done)
  • this requires a deprecation
• add a new parameter
• make the existing parameter accept a file like object as well as a path (this isn't the most preferable option as it can cause confusion with path in the parameter name.

The preferable option is to deprecate the existing parameter, rename it, and have it accept both. This shouldn't be too difficult. Take a look at the existing deprecations for some examples of how it's been done.

This should get you started. Let me know if you need any help!

The preferable option is to deprecate the existing parameter, rename it, and have it accept both. This shouldn't be too difficult. Take a look at the existing deprecations for some examples of how it's been done.

I think that is common for many libraries, but have a distaste for the variability of the type. I'd prefer to have mutually exclusive keyword arguments media_path, and media_object and use a runtime exception (or whatever we commonly use for that) if both or neither are provided. Thoughts?

but have a distaste for the variability of the type

That is a good point.

I do like the idea of adding a new parameter to the media methods (including inline media) for file like objects. My only suggestion is to do _fp instead of _object. _object feels odd and makes me think there is a PRAW type that would need to be used.

I'd prefer to have mutually exclusive keyword arguments media_path, and media_object and use a runtime exception (or whatever we commonly use for that) if both or neither are provided. Thoughts?

The original version I wrote actually supported that, though not exactly as you've described. I decided to merge both parameters into one because I wasn't sure what the preferred way of handling the issue of both types being submitted was.

So, right now the checklist looks like:

• Use BytesIO as the exclusive dependency to handle Bytes Objects
• Implement mutually exclusive keyword arguments for media_path and media_fp (not media_object?)
• Add a runtime exception if neither are provided for submit_gallery or if both are provided for submit_image (which means they are now optional parameters for both functions)

Do I have all that right?

For the most part yes.

Use BytesIO as the exclusive dependency to handle Bytes Objects

I think BinaryIO would be more appropriate since this is for media objects.

Implement mutually exclusive keyword arguments for media_path and media_fp (not media_object?)

This is still up for discussion.

Add a runtime exception if neither are provided for submit_gallery or if both are provided for submit_image (which means they are now optional parameters for both functions)

We need to consider the implications of adding new arguments with the methods that have positional arguments deprecated. i.e., if they become optional they need to be made a keyword-only argument and that might mess with another deprecation. I will help with this point as the code for _deprecate_args decorator is a bit complex.

Do I have all that right?

For the most part, yes. I think it would be best to also include the inline media, subreddit stylesheet images, and widget images as well (basically anything you can upload an image for).

media_fp works for me, as long as we're consistent (I'm not sure if file objects are passed anywhere else in praw).

Made the changes discussed here, though so far only to submit_image and submit_gallery. Both of those work, but because the changes were only made there several other functions now fail because they lack the new parameter (media_fp.) Will need to go through and adjust any functions lacking the parameter so they include it.

I removed the PIL import but couldn't figure out how to make use of BinaryIO (or even how to use it, Google was unhelpful.) Instead I decided to make use of the already included dependency of BytesIO, which proved useful enough.

With that out of the way, the core of the new implementation works as follows:

            magic_number = [int (aByte) for aByte in media_fp[:8]] # gets the format indicator

            file_headers = { 
                tuple([int (aByte) for aByte in bytes([0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A])]): 'png',
                tuple([int (aByte) for aByte in bytes([0x6D, 0x6F, 0x6F, 0x76])]): 'mov',
                tuple([int (aByte) for aByte in bytes([0x66, 0x74, 0x79, 0x70, 0x69, 0x73, 0x6F, 0x6D])]): 'mp4',
                tuple([int (aByte) for aByte in bytes([0xFF, 0xD8, 0xFF, 0xE0])]): 'jpg',  
                tuple([int (aByte) for aByte in bytes([0xFF, 0xD8, 0xFF, 0xE0, 0x00, 0x10, 0x4A, 0x46])]): 'jpeg', 
                tuple([int (aByte) for aByte in bytes([0x47, 0x49, 0x46, 0x38, 0x37, 0x61])]): 'gif',   
            }
            for size in range(4, 10, 2): # size will equal 4, 6, 8
                file_extension = file_headers.get(tuple(magic_number[:size])) 
                if file_extension is not None: 
                    mime_type = mime_type.get(file_extension, "image/jpeg") # default to JPEG
                    file_name = mime_type.split("/")[0] + "." + mime_type.split("/")[1]
                    break 

Every file format has a unique number of bytes (called a magic number since I guess the 1960s? I just learned about this while putting this together) in its header to indicate what format the file is in. This is the same every time for a format of a given type. PNG files for example contain the following decimal values (converted from hexidecimal): 137 80 78 71 13 10 26 10. Incoming byte values are converted to a list of integers and used as a key for the file_headers dictionary to pull out the file_extension value.

Otherwise, media_path has been reverted to its original functionality. Overall I think I'm going to reach a point where I plateau due to not understanding the deeper workings of PRAW--that's probably when I'll need to hand it off to Joel for final touchups--since you offered.

Thoughts?

I don't understand the point of the conversion data inside of PRAW. Anything needed to convert from the PIL image format to BytesIO objects should happen on the caller-side. It looks like you can "save" to a BytesIO file in this way: https://stackoverflow.com/questions/646286/how-to-write-png-image-to-string-with-the-pil

I don't understand the point of the conversion data inside of PRAW. Anything needed to convert from the PIL image format to BytesIO objects should happen on the caller-side. It looks like you can "save" to a BytesIO file in this way:

The idea was to avoid converting as much as possible in this case. For example, the backblaze api returns a regular encoded (hexadecimal) String. I reasoned that this would be ubiquitous among similar services and therefore the simplest type a web browser could return, which I think means it has the best chance of being the format an end-user would make use of when passing to PRAW. Otherwise the file is probably stored on their local machine, so why wouldn't they just supply the filepath at that point?

( Changing it to require an image converted from PIL (or any other library) would not be difficult; it's just a matter of deciding which input format is preferable. )

Changing it to require an image converted from PIL (or any other library) would not be difficult; it's just a matter of deciding which input format is preferable.

For the media_fp it should be an object that has a read method just like File objects and when read it returns the bytes just as a file opened in rb mode. Any conversion to get into that format should happen on the user-side code not inside of PRAW.

There's an example here of saving a PIL file to a BytesIO object: https://stackoverflow.com/a/646297

You'll likely need to seek the BytesIO object back to position 0 after saving: output.seek(0).

So upon further reviewing your last post, I'm a little confused @bboe. The link you provided talks about converting an image to a bytes string, which is already the format the current code accepts--there's no conversion going on inside of PRAW in my latest update to this PR. The current flow would be for someone to utilize the code from your Stack Overflow link, reproduced here:

import io

with io.BytesIO() as output:
    image.save(output, format="GIF")
    contents = output.getvalue()

outside of PRAW, and then they would submit the contents variable (a string) to submit_image or submit_gallery using the new media_fp parameter, like so:

reddit.subreddit("test").submit_image("title", image_fp=contents)

(I have already verified that this works using the code in its present state. I've tested it and it posts to Reddit as expected.)
It almost sounds like it already does what you're talking about--do you mind elaborating a bit?

Thanks you!

How do I run tests on PRAW without pushing new code? This is a little too slow and tedious. I'd rather just do all the testing internally before pushing, since it would be both faster and look better in the commit history...

Should be able to do so by running pytest locally in the repository root.

Okay, after some effort I've managed to ensure all the unit tests are passing. A brief summary of what that means:

• media_path and media_fp are both now considered optional parameters in the _upload_media() function.
  Prior to my PR when media_path was the only input parameter, this code handled null inputs by assigning a default value to media_path. I just reused that to handle when both media_path and media_fp are null. (This means functions like submit_video which also make use of the _upload_media() function can remain unchanged while still retaining their funtionality.)

• I added a test called "test_submit_gallery__invalid_image_fp", which can be viewed here:

   def test_submit_gallery__invalid_image_fp(self):
      subreddit = Subreddit(self.reddit, display_name="name")
  
      message = (
          "'image_fp' dictionary value at index 0 contains an invalid bytes object."
      )
      with pytest.raises(TypeError) as excinfo:
          subreddit.submit_gallery("Cool title", [{"image_fp": "invalid_image"}])
      assert str(excinfo.value) == message
  
      message = "media_fp does not represent an accepted file format (png, mov, mp4, jpg, jpeg, gif.)"
      encoded_string = "invalid_image".encode()
      with pytest.raises(TypeError) as excinfo:
          subreddit.submit_gallery("Cool title", [{"image_fp": encoded_string}])
      assert str(excinfo.value) == message
  
      message = "'NoneType' object has no attribute 'post'"
      png_image_header = [0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]
      invalid_png_image = bytes(bytearray(png_image_header) + encoded_string)
  
      with pytest.raises(AttributeError) as excinfo:
          subreddit.submit_gallery("Cool title", [{"image_fp": invalid_png_image}])
      assert str(excinfo.value) == message
  

This handles invalid objects, invalid file headers, and corrupted images being passed to the media_fp parameter. The code I've added should catch all of these and output (hopefully) descriptive error messages.

Let me know what you think. Hopefully it's getting there.

I'm taking a look at this currently.

Okay so there are a few things that are odd with our code that I think we should change prior to introducing your change.

1. _read_and_post_media should take a file pointer rather than try to open the file. This change will enable passing file-like objects to work more transparently.

2. _upload_media should not do any expected mime type checking. Instead it should only take the following arguments:

   • file_name (the name of the file, not the path)
   • fp (the File-like object)
   • mime_type (the mime type that will be sent)
   • upload_type (same as the current argument)

3. The expected mime type checking, and opening of the file should happen in another function like _prepare_media_upload which will only be used when a filename is provided. When a fileobject is provided, then it is necessary that the caller provides the mime-type.

Thoughts?

1. I believe that's already what it's doing, assuming this is the slice of code you're referencing (which explicitly includes the keyword "open"):

def _read_and_post_media(self, media_path, media_fp, upload_url, upload_data):
        response = None
        if media_path is not None and media_fp is None:
            with open(media_path, "rb") as media:
                response = self._reddit._core._requestor._http.post(
                    upload_url, data=upload_data, files={"file": media}
                )
        elif media_path is None and media_fp is not None:
            response = self._reddit._core._requestor._http.post(
                upload_url, data=upload_data, files={"file": BytesIO(media_fp)}
            )
        return response

Here it is media_path that is being opened before being sent along in the POST request, not media_fp. This functionality was not introduced by me however. It was already present. I may be misunderstanding what you mean though.

2. That's reasonable, it would just put the impetus on the end-user to provide accurate data prior to their request. I believe sending the wrong mime-type will trigger a RequestException which is currently being passed over:

try:
            upload_response = self._reddit.post(url, data=img_data)
            upload_lease = upload_response["args"]
            upload_url = f"https:{upload_lease['action']}"
            upload_data = {
                item["name"]: item["value"] for item in upload_lease["fields"]
            }
        except RequestException:
            pass

A new exception will likely be triggered in the following _read_and_post_media function. Catching it and providing an error message to the end user should be enough.

Finally, I think that since mime_type as an input parameter is only relevant as a supplement to media_fp (since the mime type of media_path is derived from the path itself) it makes more sense to convert media_fp into a dictionary like so: media_fp: Optional[Dict[bytes, str]] where a bytes key refers to a file pointer and a mime_type key refers to the mime type. This could be made clear to an end user by updating the documentation at the beginning of the submit_image and submit_gallery functions to provide clear examples of how the parameter should be provided.

3. To be clear, this would be for the mime type checking of media_path, correct? Because the preceding mime type check functionality is so small that it might not warrant an additional function, and that's all that would remain after the stripping out of the type checking for bytes objects. I'm not sure if this part is necessary on account of that.

So after reviewing the code I ultimately decided to throw out my response to point 2 and actually just go with what you recommended @bboe, it ended up being the best way forward in terms of readability and code complexity (or lack thereof!) I've removed the mime checking and replaced it with a user-supplied mime_type string.

I've added the following to try and ensure parity with media_path:

if isinstance(media_fp, bytes):
                mime_type = mime_types.get(
                    mime_type.partition("/")[1], "image/jpeg"
                )  # default to JPEG

The idea being to ensure a mime_type is always supplied which is a vestige of the old code.

Currently I'm adding little scraps of documentation inside of submit_image, submit_gallery, and upload_media so that mime_type has a documented reference for anyone who looks into the code in the future.

1. I believe that's already what it's doing, assuming this is the slice of code you're referencing (which explicitly includes the keyword "open"):

def _read_and_post_media(self, media_path, media_fp, upload_url, upload_data):
        response = None
        if media_path is not None and media_fp is None:
            with open(media_path, "rb") as media:
                response = self._reddit._core._requestor._http.post(
                    upload_url, data=upload_data, files={"file": media}
                )
        elif media_path is None and media_fp is not None:
            response = self._reddit._core._requestor._http.post(
                upload_url, data=upload_data, files={"file": BytesIO(media_fp)}
            )
        return response

Here it is media_path that is being opened before being sent along in the POST request, not media_fp. This functionality was not introduced by me however. It was already present. I may be misunderstanding what you mean though.

What @bboe is meaning is to change the existing functionality in PRAW before your changes:

Okay so there are a few things that are odd with our code that I think we should change prior to introducing your change.

To comment on what was said here, I agree with the first point. The second point, I think _upload_media could have the following signature:

• file_name (the name of the file, not the path)
• fp (the File-like object)
• upload_type (same as the current argument)

I think the mime type could be removed here because it could be derived from the file name (which is what PRAW currently does). For the third point, I do think there should be a separate function like what @bboe suggested, however, it should require file_name if a BytesIO like object is passed. If a file pointer (i.e., BufferedReader) is passed, fp should have a name attribute containing the file name.

I also wanted to add that this PR should also include the methods for uploading images for inline media, emojis, widgets, and the stylesheet.

• file_name (the name of the file, not the path)
• fp (the File-like object)
• upload_type (same as the current argument)

So would media_path still exist as a parameter? E.g., would the final function look like this:

def _upload_media(
        self,
        *,
        expected_mime_prefix: Optional[str] = None,
        path: Optional[str] = None,
        fp: Optional[bytes] = None,
        file_name: Optional[str] = None,
        upload_type: str = "link",
    ):

And to comment on the inline media/emoji/widget/stylesheet part, I'd like to nail down exactly how this part should work first. It will make copying the functionality over easier once the existing code in the PR has been finalized (submit_image, submit_gallery, _upload_media.)

To comment on this part:

it should require file_name if a BytesIO like object is passed. If a file pointer (i.e., BufferedReader) is passed, fp should have a name attribute containing the file name.

Could you provide an example of how that would differ from a BytesIO like object? Right now fp would just be an encoded String.

A BufferedReader is what is returned from with open("file_name", "r") as fp: and fp will have a name attribute with the value of file_name.

This PR is stale because it has been open for 20 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.

Going to continue working on this soon. Been otherwise occupied but I haven't abandoned this.

This PR is stale because it has been open for 20 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.

Just checking if you're still wanting this to be merged in? Is there anything we could help with?

This PR is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 30 days.

Using a BufferedReader for upload would be a welcome change.